Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding Adios2 io support #113

Merged
merged 20 commits into from
Feb 3, 2025
Merged

adding Adios2 io support #113

merged 20 commits into from
Feb 3, 2025

Conversation

seegyoung
Copy link

  • support for serial adios2 file I/O (set Omega_h_USE_ADIOS2 ON)
  • MPI only with single process at the moment
  • testing with kokkos and coda not included

@seegyoung seegyoung added the enhancement New feature or request label Sep 9, 2024
@jacobmerson
Copy link
Collaborator

/runtests

Copy link

Test Result: failure (details)

Copy link
Collaborator

@jacobmerson jacobmerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Seegyoung, from what I see so far it looks like things are on the right track.

However, it seems like there are some missing files in your commit. I don't see the Omega_h_adios2.hpp file, or where the write_adios2 code is located.

cmake/FindADIOS2.cmake Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@cwsmith
Copy link

cwsmith commented Sep 10, 2024

Thank you @seegyoung. Looks good.

If it isn't there already, and while partitioned mesh support is under development, we should add a check to the adios writer and reader to ensure that the mesh is serial.

@cwsmith cwsmith added the adios2 label Sep 10, 2024
@seegyoung
Copy link
Author

I will update based on all the input from both of you.

src/adios2_io.cpp Outdated Show resolved Hide resolved
@jacobmerson
Copy link
Collaborator

@seegyoung I still don't see the definitions for write_value and read_value for adios2.

@seegyoung
Copy link
Author

Sorry about the trouble - missing files are added

@cwsmith
Copy link

cwsmith commented Sep 17, 2024

@seegyoung Would you please remove the romulus-config.sh file? We maintain build instructions for various systems in the wiki: https://github.com/SCOREC/omega_h/wiki

@seegyoung
Copy link
Author

OK I will do.

@jacobmerson
Copy link
Collaborator

Hi Seegyoung, did you have a chance to address the comments from Cameron and I?

@seegyoung
Copy link
Author

Yes, it was removed on September 17th. Thanks for checking with me.

@jacobmerson
Copy link
Collaborator

@seegyoung can you update this pull request with your changes? I do not see the requested updates.

@seegyoung
Copy link
Author

I am not good at GitHub so I don't know what to do to resolve the issue and what's the source of the issue. Can someone can do this for me please?

@cwsmith
Copy link

cwsmith commented Dec 13, 2024

If you have changes that are committed into your local copy of the branch (i.e., on the SCOREC filesystem) you can run 'git push' to upload them to github. This PR will automatically be updated.

I'm available to discuss via WebEx if needed. Please send me an email and we can pick a time.

Copy link
Collaborator

@jacobmerson jacobmerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seegyoung i have added additional comments. Please address them and update this pull request.

cmake/FindADIOS2.cmake Outdated Show resolved Hide resolved
src/Omega_h_adios2.hpp Outdated Show resolved Hide resolved
src/adios2_io.cpp Outdated Show resolved Hide resolved
src/adios2_io.cpp Outdated Show resolved Hide resolved
src/adios2_io.cpp Outdated Show resolved Hide resolved
src/adios2_io.cpp Outdated Show resolved Hide resolved
src/adios2_io.cpp Outdated Show resolved Hide resolved
src/bp2osh.cpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.hpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.hpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Show resolved Hide resolved
src/Omega_h_adios2.cpp Show resolved Hide resolved
src/Omega_h_adios2.cpp Show resolved Hide resolved
src/Omega_h_adios2.cpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Show resolved Hide resolved
src/Omega_h_adios2.hpp Outdated Show resolved Hide resolved
Copy link

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you. A few comments are below.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Outdated Show resolved Hide resolved
src/Omega_h_adios2.cpp Outdated Show resolved Hide resolved
src/adios2_io.cpp Outdated Show resolved Hide resolved
@cwsmith
Copy link

cwsmith commented Jan 30, 2025

@seegyoung Thanks for the updates. It looks like the mesh object is passed into the [read|write]_[value|array](...) (and related functions) to extract MPI communicator info. Instead of passing the mesh there, I think it would be cleaner to pass the Omegah comm object (i.e., mesh->comm()). This makes it clear there is no 'mesh' dependency in those functions.

@seegyoung
Copy link
Author

That sounds good. I will do so.

@jacobmerson
Copy link
Collaborator

@cwsmith does Omega_h have logging or print functions that we should be using to be XSDK compliant? I didn't find anything with a quick check.

@cwsmith
Copy link

cwsmith commented Jan 31, 2025

@jacobmerson Good question. Adios2 is not a part of the xSDK (https://xsdk.info/release-1-1-0/). I did a couple of quick searches through the adios2 docs (https://adios2.readthedocs.io/en/v2.10.2/) and didn't find anything obvious.

Comment on lines +237 to +238
target_include_directories(omega_h PRIVATE "${ADIOS2_INCLUDE_DIRS}")
target_link_libraries(omega_h PUBLIC "${ADIOS2_LIBRARIES}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwsmith @seegyoung should we be using the adios2 target here? Or, do we need to handle this with the library variables because of bob.cmake?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Cameron can answer this

@jacobmerson
Copy link
Collaborator

I just had one remaining question on the CMakeLists.txt, that hopefully Cameron will know more about. Once we resolve I'm ready to merge.

@jacobmerson jacobmerson self-requested a review February 3, 2025 05:46
@seegyoung seegyoung merged commit d09d2bb into master Feb 3, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adios2 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants